Scripting: Fix Lua 5.1/LuaJIT and Lua 5.2 problems #3376

nuive · 2024-12-16T22:01:08Z

What's added:

-New option available for CMake USE_LUA option. Setting it to "JIT" will force CMake to look for LuaJIT when building.

What's fixed:

-Lua 5.1/JIT couldn't even load a script.
-Updated lua pairs/ipairs handling (Lua 5.1/JIT and 5.2 weren't able to manage things like mSTList iteration). Now pairs/ipairs work like Lua 5.4 (except for a lua_callk which stays lua_call).
-Custom objects show their propper names using Lua versions prior to 5.3.

What's not fixed:

I didn't find anything else needing a fix regarding specific Lua versions.

endrift

Tagging in @ahigerd for this. I'm not sure why some of these changes were made, and I'm concerned about the provenance of some of the code.

endrift · 2024-12-22T08:35:32Z

src/platform/cmake/FindLuaJIT.cmake

What is the provenance of this file? There's no info in the file itself, and it looks like it comes from somewhere

It looks like it's coming mostly from CMake's FindLua.cmake file, but it's changed kind of weirdly for LuaJIT. It appears to be mostly just find and replace lua with luajit and such, then some changes to handle LuaJIT versions (2.1 and 2.0?), and there's kind of weird remanents remaining with lua functions getting a luajit replacement, but then also leaving in the older functions. The license is removed for some reason, and even a comment mentioning FindLua was removed here for some reason.

Sorry, it's bassed on CMake FindLua.cmake. I forgot to restore/modify comments.

The old Lua functions are there because of library name (lua51). I left them thinking LuaJIT could be built using different Lua versions in the future, but nowadays they could be removed and replaced those calls with "lua51" or "lua5.1" constants.

LuaJIT will never be anything but 5.1, as directly stated by Mike Pall -- the fenv change is too compatibility-breaking for the JIT core to ever work with 5.2+.

It might be interesting to someday consider supporting Ravi as a 5.2+ JIT engine, but I don't know what kind of API compatibility Ravi has or if it's even worth it since Ravi is much heavier than LuaJIT.

endrift · 2024-12-22T08:45:24Z

src/script/engines/lua.c

+const char* luaL_tolstring (lua_State* lua, int idx, size_t* len) {
+	if (luaL_callmeta(lua, idx, "__tostring")) {
+		if (!lua_isstring(lua, -1)) {
+			luaL_error(lua, "'__tostring' must return a string");
+		}
+	} else {
+		switch (lua_type(lua, idx)) {
+			case LUA_TNUMBER:
+			case LUA_TSTRING:
+				lua_pushvalue(lua, idx);
+				break;
+			case LUA_TBOOLEAN:
+				lua_pushstring(lua, (lua_toboolean(lua, idx) ? "true" : "false"));
+				break;
+			case LUA_TNIL:
+				lua_pushliteral(lua, "nil");
+				break;
+			default:
+				luaL_getmetafield(lua, idx, "__name");
+				int tt = lua_type(lua, -1);
+				const char* name = (tt == LUA_TSTRING) ? lua_tostring(lua, -1) :
+				                                         luaL_typename(lua, idx);


Don't copy source code directly from Lua, please. The license may permit this, but it'd be much better to replace the one call to this function with something like what Lua 5.1 does in its print function: it just calls tostring on the argument directly to coerce it.

Ironic: My print() shim calls luaL_tostring explicitly to avoid doing that.

That said, 5.1's tostring doesn't honor __name, so it becomes a question for @endrift: Is the better debugging/introspection worth the extra work?

endrift · 2024-12-22T08:46:09Z

src/script/engines/lua.c

+static int _luaPairs(lua_State* lua) {
+	luaL_checkany(lua, 1);
+	if (luaL_getmetafield(lua, 1, "__pairs") == LUA_TNIL) {
+		lua_getglobal(lua, "next");
+		lua_pushvalue(lua, 1);
+		lua_pushnil(lua);
+	} else {
+		lua_pushvalue(lua, 1);
+		lua_call(lua, 1, 3);
+	}
+	return 3;
+}
+
+static int _luaIpairsIter(lua_State* lua) {
+	lua_Integer i = luaL_checkinteger(lua, 2) + 1;
+	lua_pushinteger(lua, i);
+	lua_pushvalue(lua, -1);
+	lua_gettable(lua, 1);
+	return (lua_type(lua, -1) == LUA_TNIL) ? 1 : 2;
+}
+
+static int _luaIpairs(lua_State* lua) {
+	luaL_checkany(lua, 1);
+	lua_pushcfunction(lua, _luaIpairsIter);
+	lua_pushvalue(lua, 1);
+	lua_pushinteger(lua, 0);
+	return 3;
+}


Isn't pairs/ipairs only in 5.2/5.3, not 5.1?

No, they're in 5.1. You're thinking of the metamethods.

That's right. In fact these modifications add __pairs metamethod to Lua 5.1, and removes the LUA_TTABLE type restriction for pairs/ipairs.

What's the use case? (And if you're implementing this, why not add __ipairs too?)

-Lua 5.1: pairs and ipairs work only with table objects. Also they don't check for __ respective metamethods.
-Lua 5.2: pairs and ipairs work only with table objects. They check for __ respective metamethods.
-Lua 5.3: pairs check for __pairs metamethod. ipairs check for __ipairs metamethod only in compatibility mode. Object type is irrelevant (only checks if its a table when using luaB_next iterator (the global next function).
-Lua 5.4: Only pairs check for its __ metamethod. Like in 5.3 object type is irrelevant (only checks if its a table when using luaB_next iterator (the global next function).

So the approach is done with Lua 5.4's functionality in mind.

It's used for mSTList iteration and mSTTable __pairs metamethod.

Oops... You're right!

I'll fix that.

Okay, this seems different... I think there's no easy way of overriding # operator, neither is a replaceable function like pairs and ipairs.

You're right, there isn't, but if you're going to provide the 5.4 semantics for ipairs then you can check for the metamethod manually.

I think I'm not getting this... What connection exists between __len and ipairs? ipairs in Lua 5.4 is different from Lua 5.1, but I think I've adjusted it propperly in _luaIpairsIter and _luaIpairs functions.

checks documentation Oh. Huh. I was under the impression that ipairs was semantically equivalent to for i = 1, #t do in 5.4 since the __ipairs metamethod was removed, but apparently I was mistaken.

endrift · 2024-12-22T08:46:36Z

src/script/engines/lua.c

@@ -226,6 +286,7 @@ static const int _mScriptSocketNumErrors = sizeof(_mScriptSocketErrors) / sizeof
 #define lua_pushglobaltable(L) lua_pushvalue(L, LUA_GLOBALSINDEX)
 #endif

+


Please don't add a stray newline

endrift · 2024-12-22T08:47:03Z

src/script/engines/lua.c

+	lua_pushcfunction(luaContext->lua, _luaIpairs);
+	lua_setglobal(luaContext->lua, "ipairs");
+
+	lua_pushcfunction(luaContext->lua, _luaPairs);
+	lua_setglobal(luaContext->lua, "pairs");


Aren't pairs/ipairs builtins in 5.2?

They're builtins in 5.1 and LuaJIT so I'm not sure what this is for.

These lines are added in order to replace the original functions with the ones defined in this file. I don't know if there's a better method for this.

The only thing that comes to mind is that PUC-Rio had removed ipairs from prerelease versions of 5.2 before re-adding it in response to overwhelming outcry from the community. If this code is derived from some ancient 5.1/5.2 compatibility hack then that might explain it, but pairs was never on the chopping block so I really don't know. Never mind, question was answered.

endrift · 2024-12-22T08:50:59Z

src/script/engines/lua.c

+#if LUA_VERSION_NUM >= 502
 		lua_getupvalue(luaContext->lua, -4, 1);
+#else
+		lua_getfenv(luaContext->lua, -4);
+#endif


lua_getupvalue exists with the same signature in 5.1. What's different in 5.1 that this is needed?

This is the breaking change between 5.1 and 5.2+ -- in 5.2+ the first upvalue of a function is the lexical environment (if it exists -- you cannot assume a function inherits a lexical environment), but in 5.1 the lexical environment is managed by getfenv/setfenv.

ahigerd · 2024-12-23T15:32:09Z

CMakeLists.txt

@@ -768,7 +768,9 @@ endif()
 if(ENABLE_SCRIPTING)
 	list(APPEND ENABLES SCRIPTING)
 	find_feature(USE_JSON_C "json-c")
-	if(NOT USE_LUA VERSION_LESS 5.1)
+	if(USE_LUA STREQUAL "JIT")
+		find_feature(USE_LUA "Lua${USE_LUA}")


Might as well just write LuaJIT here since it's guaranteed to evaluate to that after the previous line.

ahigerd · 2024-12-23T15:33:41Z

src/platform/cmake/FindLuaJIT.cmake

+unset(_luajit_append_versions)
+
+# this is a function only to have all the variables inside go away automatically
+function(_lua_get_versions)


This is irrelevant for LuaJIT as it always acts like 5.1.

I believe that's not true for LuaJIT 2.1, but also this file has a lot of stuff that definitely mishandles the versioning as it expects Lua versions

LuaJIT 2.1 still always acts like 5.1 in terms of semantics; it just supports some 5.2+ features as extensions.

That's because inside LuaJIT are defined LUAJIT_VERSION (2.x) and LUA_VERSION (5.1). LUA_VERSION is used into the library name. I've simplified the cmake file. As previously said LuaJIT'll always be Lua 5.1, all non LuaJIT references will be removed once I commit hte new changes.

ahigerd · 2024-12-23T15:38:55Z

src/platform/cmake/FindLuaJIT.cmake

+    return()
+  endif ()
+
+  # At least 5.[012] have different ways to express the version


We know that it's 5.1, so simplify this.

ahigerd · 2024-12-23T15:47:09Z

src/script/engines/lua.c

@@ -390,6 +451,8 @@ struct mScriptEngineContext* _luaCreate(struct mScriptEngine2* engine, struct mS

 	luaL_newmetatable(luaContext->lua, "mSTStruct");
 #if LUA_VERSION_NUM < 502
+	lua_pushliteral(luaContext->lua, "mSTStruct");


I become tempted to refactor these to reduce just how much repetition is involved here, but that's kinda out of scope for this PR.

Yes, I'm definitely missing something here. I agree with this needing refactorization but I don't know how at the momment.

void _luaNewMetatable(lua_State* lua, const char* name, luaL_Reg* funcs) { luaL_newmetatable(lua, name); /* ... etc ... */ lua_pop(lua); }

_luaNewMetatable(luaContext->lua, "mSTStruct", _mSTStruct);

(I guess that should probably be declared static.)

ahigerd · 2024-12-23T17:40:45Z

I think the implementation of _luaLoad is missing some fenv fixes.

nuive · 2024-12-23T20:39:59Z

I think the implementation of _luaLoad is missing some fenv fixes.

Like what?

ahigerd · 2024-12-23T21:06:18Z

I think the implementation of _luaLoad is missing some fenv fixes.

Like what?

      // Create new _ENV
      lua_newtable(luaContext->lua);
  
      // Make the old _ENV the __index in the metatable
      lua_newtable(luaContext->lua);
      lua_pushliteral(luaContext->lua, "__index");
      lua_getupvalue(luaContext->lua, -4, 1);
      lua_rawset(luaContext->lua, -3);
  
      lua_pushliteral(luaContext->lua, "__newindex");
      lua_getupvalue(luaContext->lua, -4, 1);
      lua_rawset(luaContext->lua, -3);
  
      lua_setmetatable(luaContext->lua, -2);
  
      lua_pushliteral(luaContext->lua, "script");
      lua_newtable(luaContext->lua);

This is _ENV manipulation which means it needs to be handled using setfenv instead. It's probably a little nontrivial to do correctly but if you don't do it then require() won't behave correctly.

nuive · 2024-12-24T00:45:35Z

      // Create new _ENV
      lua_newtable(luaContext->lua);
  
      // Make the old _ENV the __index in the metatable
      lua_newtable(luaContext->lua);
      lua_pushliteral(luaContext->lua, "__index");
      lua_getupvalue(luaContext->lua, -4, 1);
      lua_rawset(luaContext->lua, -3);
  
      lua_pushliteral(luaContext->lua, "__newindex");
      lua_getupvalue(luaContext->lua, -4, 1);
      lua_rawset(luaContext->lua, -3);
  
      lua_setmetatable(luaContext->lua, -2);
  
      lua_pushliteral(luaContext->lua, "script");
      lua_newtable(luaContext->lua);

This is _ENV manipulation which means it needs to be handled using setfenv instead. It's probably a little nontrivial to do correctly but if you don't do it then require() won't behave correctly.

Yes, I know. In fact mGBA built with Lua 5.1 breaks if this is not handled propperly.

However if you look at the submited changes there's this:

		// Create new _ENV
		lua_newtable(luaContext->lua);

		// Make the old _ENV the __index in the metatable
		lua_newtable(luaContext->lua);
		lua_pushliteral(luaContext->lua, "__index");
#if LUA_VERSION_NUM >= 502
		lua_getupvalue(luaContext->lua, -4, 1);
#else
		lua_getfenv(luaContext->lua, -4);
#endif
		lua_rawset(luaContext->lua, -3);

		lua_pushliteral(luaContext->lua, "__newindex");
#if LUA_VERSION_NUM >= 502
		lua_getupvalue(luaContext->lua, -4, 1);
#else
		lua_getfenv(luaContext->lua, -4);
#endif
		lua_rawset(luaContext->lua, -3);

		lua_setmetatable(luaContext->lua, -2);

		lua_pushliteral(luaContext->lua, "script");
		lua_newtable(luaContext->lua);

		if (dirname[0]) {
			lua_pushliteral(luaContext->lua, "require");
			lua_pushstring(luaContext->lua, dirname);
			lua_pushcclosure(luaContext->lua, _luaRequireShim, 1);
			lua_rawset(luaContext->lua, -5);

			lua_pushliteral(luaContext->lua, "dir");
			lua_pushstring(luaContext->lua, dirname);
			lua_rawset(luaContext->lua, -3);
		}

		if (name[0] == '@') {
			lua_pushliteral(luaContext->lua, "path");
			lua_pushstring(luaContext->lua, &name[1]);
			lua_rawset(luaContext->lua, -3);
		}

		lua_rawset(luaContext->lua, -3);
#if LUA_VERSION_NUM >= 502
		lua_setupvalue(luaContext->lua, -2, 1);
#else
		lua_setfenv(luaContext->lua, -2);
#endif
		luaContext->func = luaL_ref(luaContext->lua, LUA_REGISTRYINDEX);
		return true;

Woudln't this be enough? Am I missing any (set/get)fenv?

ahigerd · 2024-12-24T00:58:37Z

Hmm... I may have misread the code. That might be correct after all.

nuive · 2024-12-28T20:02:29Z

I think I made all requested changes...

endrift requested changes Dec 22, 2024

View reviewed changes

ahigerd reviewed Dec 23, 2024

View reviewed changes

nuive force-pushed the master branch from 2afea18 to d48a3d9 Compare December 28, 2024 19:56

Scripting: Fix Lua 5.1/LuaJIT and Lua 5.2 problems

964f505

nuive force-pushed the master branch from d48a3d9 to 964f505 Compare December 28, 2024 20:00

		@@ -226,6 +286,7 @@ static const int _mScriptSocketNumErrors = sizeof(_mScriptSocketErrors) / sizeof
		#define lua_pushglobaltable(L) lua_pushvalue(L, LUA_GLOBALSINDEX)
		#endif

Scripting: Fix Lua 5.1/LuaJIT and Lua 5.2 problems #3376

Are you sure you want to change the base?

Scripting: Fix Lua 5.1/LuaJIT and Lua 5.2 problems #3376

Conversation

nuive commented Dec 16, 2024

endrift left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuive Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahigerd Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahigerd Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahigerd commented Dec 23, 2024

nuive commented Dec 23, 2024

ahigerd commented Dec 23, 2024

nuive commented Dec 24, 2024

ahigerd commented Dec 24, 2024

nuive commented Dec 28, 2024

nuive Dec 24, 2024 •

edited

Loading

ahigerd Dec 23, 2024 •

edited

Loading

ahigerd Dec 23, 2024 •

edited

Loading